Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add receive:append permission for limited receive #17015

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shodanshok
Copy link
Contributor

Force receive (zfs receive -F) can rollback or destroy snapshots and file systems that do not exist on the sending side (see zfs-receive man page). This means an user having the receive permission can effectively delete data on receiving side, even if such user does not have explicit rollback or destroy permissions.

This patch adds the append permission, which only permits limited, non-forced receive. Behavior for users with full receive permission is not changed in any way.

Fixes #16943

Motivation and Context

Description

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I like the permission named just "append", not some "receive:append" or something like that, what is allowed by the command syntax. Also as I have told before, it would be nice to investigate other related cases, like receive with rollback to the last snapshot, and think about similar permissions for the rollback command.

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Feb 3, 2025
@shodanshok
Copy link
Contributor Author

I don't think I like the permission named just "append", not some "receive:append" or something like that, what is allowed by the command syntax.

I changed the permission name to receive:append as suggested, thanks.

Also as I have told before, it would be nice to investigate other related cases, like receive with rollback to the last snapshot, and think about similar permissions for the rollback command.

While true, I feel this would significantly increase the scope of this patch, which really is about providing a simple solution to the security issue we have now (and between a module tunable and this new permission I prefer the latter).

@shodanshok shodanshok changed the title Add append permission for limited receive Add receive:append permission for limited receive Feb 8, 2025
@behlendorf
Copy link
Contributor

After catching up on the discussion in #16991 this does seem like the best way forward. As part of this PR can you'll need to extend the delegation tests accordingly, tests/zfs-tests/tests/functional/delegate/*. Let's also add a specific test there to verify the append behavior is working as intended and doesn't allow forced receive rollbacks.

@shodanshok
Copy link
Contributor Author

After catching up on the discussion in #16991 this does seem like the best way forward. As part of this PR can you'll need to extend the delegation tests accordingly, tests/zfs-tests/tests/functional/delegate/*. Let's also add a specific test there to verify the append behavior is working as intended and doesn't allow forced receive rollbacks.

I added a basic test. This is my first try with the test suite, I hope the test is working as expected.

@shodanshok shodanshok force-pushed the permission branch 2 times, most recently from 1a31166 to 721c5e3 Compare February 26, 2025 00:11
log_must_busy zfs destroy -rf $dtst

log_must zfs allow $user create,mount,canmount $fs
user_run $user eval "zfs receive -o canmount=off -F $dtst < $bak_user"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could/chould we check for error here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure, as the receive test (which I copied) does not check for such errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked how to do it, but I find weird that we verify command error not by its explicit exit status, but by expected result of its work. Would be good to check both.

@amotin
Copy link
Member

amotin commented Mar 5, 2025

@shodanshok Please rebase this to the latest master, or it won't pass CI tests on Fedora due to their kernel update.

Force receive (zfs receive -F) can rollback or destroy snapshots and
file systems that do not exist on the sending side (see zfs-receive man
page). This means an user having the receive permission can effectively
delete data on receiving side, even if such user does not have explicit
rollback or destroy permissions.

This patch adds the receive:append permission, which only permits
limited, non-forced receive. Behavior for users with full receive
permission is not changed in any way.

Fixes openzfs#16943

Signed-off-by: Gionatan Danti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incremental receives can destroy snapshots without the destroy permission
4 participants